Conversation
NickLarsenNZ
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
NickLarsenNZ
left a comment
NickLarsenNZ
left a comment
There was a problem hiding this comment.
Left a couple of comments about namespace deletion.
Techassi
left a comment
Techassi
left a comment
There was a problem hiding this comment.
Left a few comments and questions.
This reverts commit 7dfe50e.
Co-authored-by: Techassi <git@techassi.dev>
Techassi
left a comment
Techassi
left a comment
There was a problem hiding this comment.
Only a few small comments left, mostly looks nice!
| }; | ||
|
|
||
| for object in object_list { | ||
| if let Some(value) = object.labels().get(&label.key().to_string()) { |
There was a problem hiding this comment.
note: This is kinda cumbersome to use, but could be improved by stackabletech/operator-rs#1182 (if we decide to move forward with it).
There was a problem hiding this comment.
The linked PR was merged btw, but not yet released.
| let proceed_with_uninstall = tracing_indicatif::suspend_tracing_indicatif( | ||
| || -> Result<bool, CmdError> { | ||
| Confirm::new() | ||
| .with_prompt( | ||
| format!( | ||
| "Uninstalling the demo {demo_name:?} will delete the {demo_namespace:?} namespace. This action cannot be undone. Proceed?", | ||
| demo_name = args.demo_name.clone(), | ||
| demo_namespace = args.namespaces.namespace.clone()) | ||
| ) | ||
| .default(true) | ||
| .interact() | ||
| .context(ConfirmDialogSnafu) | ||
| }, | ||
| )?; |
There was a problem hiding this comment.
Curious what happens when this runs in a script? Does indicative choose something, or just hang?
There was a problem hiding this comment.
Tried it out, this would result in an error:
An unrecoverable error occurred: failed to execute demo (sub)command
Caused by these errors (recent errors listed first):
1: failed to confirm user input
2: IO error: No such device or address (os error 6)
There is the .interact_opt() function, which theoretically might return None if it's not a terminal the command is running in. But it would also return a None if the user presses Esc/q to cancel, but those two should behave differently, so can't distinguish based on that.
I think your suggestion of having a --yes/-y option would be the way to go here.
3 participants
Description
Part of #187
This PR adds the
uninstallsubcommands todemoandstackcommands. It implements the MVP features mentioned here #187 (comment)The flag of skipping operators and CRD deletion was optional and I wasn't sure about the naming or if needs to be one command or two. Can also be just removed, added it because it was not much additional work.
Tested with all current demos/stacks. Some demos have edge cases in the deletion, will document that in the parent issue.
This PR also contains thestripped out into #432DEMO/STACKparameter addition for stackabletech/demos#374 (comment)Definition of Done Checklist
Author
Reviewer
Acceptance